ot_usbdev: Create initial usbdev block
#180
Conversation
pamaury
force-pushed
the
usbdev
branch
2 times, most recently
from
b3d7d26 to
49df0bb
Compare
pamaury
force-pushed
the
usbdev
branch
3 times, most recently
from
8048aee to
48bf161
Compare
pamaury
changed the title
pamaury
force-pushed
the
usbdev
branch
2 times, most recently
from
6f5515e to
521a3c0
Compare
pamaury
force-pushed
the
usbdev
branch
3 times, most recently
from
3d5a51b to
7c18b80
Compare
pamaury
force-pushed
the
usbdev
branch
3 times, most recently
from
6e48879 to
ed93075
Compare
|
Note: it seems that PR title format should now be looking at the recent PRs. |
pamaury
force-pushed
the
usbdev
branch
2 times, most recently
from
517c204 to
d1a35a7
Compare
pamaury
changed the title
ot_usbdev: Create initial usbdev block
| static void ot_usbdev_set_vbus_sense(OtUsbdevState *s, bool sense) | ||
| { | ||
| s->vbus_sense = sense; | ||
| /* @todo need to update the link state and trigger some events here */ |
There was a problem hiding this comment.
Aside: Maybe also trace here? This can be deferred until the actual implementation of the TODO though I think.
| /* @todo cancel everything here. */ | ||
|
|
||
| memset(s->regs, 0, sizeof(s->regs)); | ||
| s->link_state = OT_USBDEV_LINK_STATE_DISCONNECTED; |
There was a problem hiding this comment.
Assuming it is intentional, could you add a short comment here like
/* s->vbus_sense should survive reset */
There was a problem hiding this comment.
Yes it's intentional, vbus sense is an external signal so there is not reason to reset it. That being said, VBUS handling will become a lot more complicated as we add more features so all of this code will chaneg anyway
| memset(s->regs, 0, sizeof(s->regs)); | ||
| s->link_state = OT_USBDEV_LINK_STATE_DISCONNECTED; | ||
|
|
||
| ot_usbdev_update_irqs(s); |
There was a problem hiding this comment.
Probably also ot_usbdev_update_alerts(s); here? Only alert test is implemented for now so it is not incorrect, but this would help prevent potential reset bugs when alerts are introduced to the USB device.
There was a problem hiding this comment.
I am not sure why alerts would need to be updated here? Alerts are more like pulses: when you write a 1, the alert is fired but it's not a level interrupt, it's never left at level 1, and resetting the block does not trigger an alert either. I am missing something about alerts?
There was a problem hiding this comment.
Maybe this is an issue with current QEMU alert modelling (which is already inconsistent). I think that currently we model fatal alerts as a level interrupt, perhaps that needs to be changed. For sticky (fatal) alerts, how does HW handle this? Does it send repeated pulses?
There was a problem hiding this comment.
I discussed with Emmanuel and he said that alerts are not level interrupts because the alert handler does not clear them, and the IPs have no way of clearing it anyway. I followed what was done in the CSRNG driver of calling ibex_irq_set(&irq, 1) followed by ibex_irq_set(&irq, 0).
There was a problem hiding this comment.
My understanding (please correct me if incorrect) is that fatal alerts (unlike e.g. alert test which is transient, as your current code is) are treated as a level interrupt and stay asserted. In the alert handler, when an alert is handled, it should subsequently check if the alert is still asserted (i.e. fatal) and if so re-trigger the alert.
Maybe I'm misunderstanding and/or the alert handler in QEMU doesn't emulate this properly, but based on this understanding, fatal device alerts should stay high until reset, at which point we should ot_usbdev_update_alerts(s) to reset them back to 0?
There was a problem hiding this comment.
This is related to what I was trying to explain in another comment on the PR: my code will never throw a real alert (the only source of alerts in the RTL is a TL-UL integrity violation) so literally the only source of alert is the ALERT_TEST register.
There was a problem hiding this comment.
On a related note: would it be better to have a different type for alerts and explicitely handle alert tests versus real alerts? Since this problem will happen in all drivers, it would be better to have generic infrastructure for that.
There was a problem hiding this comment.
No sure to get it, sorry: different types where?
There was a problem hiding this comment.
Alerts are modelled as IbexIRQ, maybe we should have a IbexAlert?
There was a problem hiding this comment.
Ah ok. Could be. IbexIRQ is only an optimization to avoid propagatin no-change again and again, but having an OtAlert type could help readibility. IbexIRQ is not OtIRQ as we were planning to use it for ibexdemo, but I do not think we made it.
Signed-off-by: Douglas Reis <[email protected]> Signed-off-by: Amaury Pouly <[email protected]>
Replaces #164. This creates a minimal implementation of the usbdev block. Currently, the only thing supported are interrupts and also VBUS handling through a dedicated chardev channel. This allows on to pass the
usbdev_vbus_test(after some opentitantool changes). Since the usbdev driver will be quite complicated, I think it's better to start small and build up. I also plan to make it multi-file so I created a directory for it.